-
Notifications
You must be signed in to change notification settings - Fork 18
feat: add can_stand_alone flag to publishable entity [FC-0083] #289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add can_stand_alone flag to publishable entity [FC-0083] #289
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
This PR simply adds |
e02cad8 to
35b338a
Compare
It allows us to track components that were created independently and components that were created under a container like unit or subsection.
35b338a to
b68d8ea
Compare
rpenido
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thank you for your work, @navinkarkera!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
Should we also bump the version here, or should we wait for more changes since it is not being used yet?
Maybe we can bump the version together with #287. CC @pomegranited |
| null=True, | ||
| blank=True, | ||
| ) | ||
| can_stand_alone = models.BooleanField(default=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera I think we need to add a comment to explain this field. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please -- could you put it in the field help_text so it's visible in Django Admin too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pomegranited
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested some more documentation on this new field, but the code is working as expected. Thank you @navinkarkera !
@ormsbee could you take a look to make sure this came through as intended? I admit I don't really understand the use case for this field, so want to make sure we're doing what is needed.
| null=True, | ||
| blank=True, | ||
| ) | ||
| can_stand_alone = models.BooleanField(default=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please -- could you put it in the field help_text so it's visible in Django Admin too?
Reading the Slack thread took me a while, so I want to summarize and check if my understanding is correct. 😬 For Containers (different from Collections), we can create an Item inside it that is not used anywhere else. So, if we delete the container, we will probably also want to remove these items. |
|
To clarify, the current product stance (at least for the Teak timeline) is that deleting a container never deletes its children. There is a concern that even if it seems more intuitive in certain situations, it will add too much user-side complexity. This will be reevaluated after some user testing. On the technical side, we're just recording this so that we have the necessary model data stored to make this distinction later if it becomes relevant. |
pomegranited
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Perfect, thank you @navinkarkera
- I tested this using the PR test instructions
- I read through the code
-
I checked for accessibility issues by using my keyboard to navigateN/A - Includes documentation
- User-facing strings are extracted for translation
It allows us to track components that were created independently and components that were created under a container like unit or subsection.
Related to: #284
Private-ref: FAL-4107Test instructions:
tutor images build openedx-dev && tutor dev launch -I --skip-build"openedx_learning.apps.authoring.units"inINSTALLED_APPSin bothcms/envs/common.pyandlms/envs/common.pyfiles.